Skip to content

motion-logger: Do not use value passed unchecked to access array#3829

Closed
smoe wants to merge 1 commit intoLinuxCNC:2.9from
smoe:motion_check_joints
Closed

motion-logger: Do not use value passed unchecked to access array#3829
smoe wants to merge 1 commit intoLinuxCNC:2.9from
smoe:motion_check_joints

Conversation

@smoe
Copy link
Collaborator

@smoe smoe commented Feb 24, 2026

Please kindly have an extra eye on me setting <,<=,> correctly. Maybe these checks are not required because those checks are already performed elsewhere. Still, it feels weird to not perform such checks. The major question though is if enough is done if any such invalid number is observed.

@smoe smoe added the for-discussion-only This pull request is intended as a basis for discussion, not to be merged as-is label Feb 24, 2026
@BsAtHome
Copy link
Contributor

IIRC, axis checks should also check the mask because you have, for example, XZA configs where Y-axis is effectively invalid. But you need to have access to the axis_mask in trajectory stat, emcStatus->motion.traj.axis_mask, which holds the valid axis as a bit mask. Don't know if that is exposed somehow, somewhere. If not, then the index check should suffice. Writing to unused axes should have no effect if the kinematics play nice.

The checks could also be slightly simplified as:

// valid: zero inclusive, max exclusive
if (joint >= 0 && joint < EMCMOT_MAX_JOINTS) {
  // valid, use joint as index
} else {
  log_print("...invalid joint index %d. Valid range [0,%d]\n", joint, EMCMOT_MAX_JOINTS-1);
}

And, why do you split lines on closing curly brace and an else clause? You'd normally write that as:

if (bla) {
  something();
} else {  // <-- this line
  something_else();
}

@smoe
Copy link
Collaborator Author

smoe commented Feb 25, 2026

Wrt formatting I still hope this will soon get resolved in an automated manner. In R, I do as you suggested, in C/C++ not :-)

I do not know how that logger is ultimately used. But from looking over that file I tend to get the impression that it should be autogenerated from some abstract description of all these commands.

@BsAtHome
Copy link
Contributor

I do not know how that logger is ultimately used.

There are at least a couple of tests that check using motion-logger (see tests/motion-logger/*).

But from looking over that file I tend to get the impression that it should be autogenerated from some abstract description of all these commands.

While I agree in principle, the practicalities are not necessarily on our side. The whole emcmot* stuff is used in RT/non-RT communication. You can't just change the interface without changing the code on both sides of the interface. So, any generated stuff needs careful manual work on both sides to fit again.

@smoe
Copy link
Collaborator Author

smoe commented Feb 25, 2026

The translations apparently do not build on bullseye because of an outdated po4a.
"configure: WARNING: po4a version too old, need version 0.67 or newer, not building translated docs"

If it is not ultimately urgent then I suggest to backport the latest po4a to bullseye-backports. And then we see - should be doable.

@rmu75
Copy link
Collaborator

rmu75 commented Mar 1, 2026

And, why do you split lines on closing curly brace and an else clause? You'd normally write that as:

if (bla) {
  something();
} else {  // <-- this line
  something_else();
}

"normally" is debatable ;-) I prefer the else on a separate line, same with try/catch statements.

in linuxcnc you can find both styles, even in the same file, but } else { seems to be predominant.

@andypugh
Copy link
Collaborator

andypugh commented Mar 1, 2026

Limits already checked in task (calling function). Further tests not needed.

@andypugh andypugh closed this Mar 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

for-discussion-only This pull request is intended as a basis for discussion, not to be merged as-is

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants